refactor(engine): add fingerprinted browser leases#2159
Conversation
1dd7199 to
82cdfe7
Compare
a9e5a31 to
8233fbc
Compare
5f2aacb to
489aa3c
Compare
164a96f to
da86e33
Compare
489aa3c to
78ee37c
Compare
da86e33 to
75ea6e2
Compare
78ee37c to
263a1a1
Compare
75ea6e2 to
d60af99
Compare
406ba14 to
a53a699
Compare
d60af99 to
6fa41a0
Compare
a53a699 to
ae6769c
Compare
3e52184 to
703384e
Compare
ae6769c to
3335e20
Compare
703384e to
83eb853
Compare
3335e20 to
a1743df
Compare
83eb853 to
46506e3
Compare
a1743df to
446f86f
Compare
46506e3 to
f4c0202
Compare
446f86f to
248ec97
Compare
f4c0202 to
e1c07c4
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
The immutable fingerprint and entry state machine are strong: browserLeasePool.ts:44-59 snapshots every launch-defining input into one canonical key, and browserLeasePool.ts:205-222 removes a closing generation from availability before awaiting teardown. The exact-identity Studio migration at studioServer.ts:167-223 also closes the previously reported cross-owner release race.
-
blocker -
packages/engine/src/services/browserManager.ts:459-466/packages/engine/src/services/browserLeasePool.ts:110-114: the still-publicreleaseBrowser(browser)API now silently does nothing for every browser returned by pooledacquireBrowser(). That API remains exported from@hyperframes/engine, its signature still acceptsBrowser, and the engine README still teaches callers to release the acquired browser through it; existing consumers therefore continue to compile but leak the lease/browser. Exact lease identity is the right new invariant, but silently returningfalsebehind aPromise<void>is not a safe compatibility path. Preserve the unambiguous legacy case (for example, release the sole active lease and fail loudly when multiple owners make a browser handle ambiguous), or make the breaking contract explicit and migrate/document it with a regression that proves old callers cannot silently leak. -
blocker -
packages/engine/src/services/frameCapture.ts:1042-1056: construction rollback awaitspage.close()without the 5-second close guard already required by normal session teardown. If page bootstrap fails afternewPage()and Chromium never settlespage.close()--the exact failure classwaitForCloseWithTimeouthandles atframeCapture.ts:3402-3409--the catch block never reachesbrowserLease.release(), socreateCaptureSession()hangs and retains the lease indefinitely. Put lease release in a guaranteedfinallypath and apply the same timeout/force-release policy; pin it with a never-resolvingpage.close()regression.
Freshness note: the reviewed head is 7d4534e08; CI is 45 successful / 0 pending / 0 failed, but main advanced to dbb684fa6 during review. The merge is conflict-free, though this repository will need the normal restack/fresh-CI cycle after these fixes.
Verdict: REQUEST CHANGES
Reasoning: The fingerprinted lease mechanism fixes the original sharing race, but one public release path silently loses ownership and the new construction rollback can hang before releasing it; both are resource-lifetime violations in the contract this PR introduces.
-- Magi
|
Addressed both ownership blockers on exact head
Validation: focused ownership suite 38/38; full engine suite 1,092 passed / 3 skipped; engine typecheck + build; changed-file lint + format; pre-commit gates; and full workspace build on the exact restacked top branch. PRs #2159-#2174 are republished and fresh CI is running. |
miguel-heygen
left a comment
There was a problem hiding this comment.
[P1] packages/engine/src/services/frameCapture.ts:1069-1074 still cannot force-close a browser whose graceful close hangs. browserLease.release() synchronously deactivates the lease before awaiting entry.closePromise (browserLeasePool.ts:194-207). When the 5s guard expires, browserLease.forceRelease() calls deactivate(true), but active is already false, so it returns at line 195 and never invokes the pool force-close path. The warning says the process is being forced down, but the Chromium process remains alive and the pool close promise remains unresolved. The new regression only covers a never-resolving page.close(); it does not exercise a never-resolving browser.close(), which is the branch at 1069-1074. Please make force release able to escalate an already-started graceful close (or force-close the browser handle after timeout) and add the corresponding hung-browser.close() regression. The same lease-level issue also affects normal closeCaptureSession, which uses the same release-then-force pattern.
The sole-owner releaseBrowser(browser) compatibility fix is correct, and the page-close timeout test closes the first blocker. This remaining escalation hole is the same resource-lifetime invariant on the next cleanup step.
Verdict: REQUEST CHANGES
Reasoning: Exact-handle ownership is repaired, but a hung graceful browser close still makes the advertised force-release path a no-op, leaving Chromium alive after the timeout.
— Magi
|
Addressed the remaining hung-
Local validation on the latest restack: ownership 39/39; full engine 1,093 passed / 3 skipped; CLI download timeout 1/1; talking-head media contract 1/1; skill lint and manifest checks clean; full workspace build passed on the exact restacked top branch. PRs #2159–#2174 are republished and fresh CI is running. |
miguel-heygen
left a comment
There was a problem hiding this comment.
Fresh exact-head re-review of d4cfa08cb646270a139ab37bde99e21b7fd49cc3 on main@3bb26b0f0 (behind_by: 0).
The remaining ownership blocker is closed. browserLeasePool.ts:208-216 now lets an inactive final lease escalate the graceful close it already started, while the entry.refCount === 0 gate prevents that stale lease from killing a browser still held by another owner. browserLeasePool.ts:224-255 turns that escalation into a one-shot signal that force-closes Chromium and resolves the pending close/entry cleanup. The construction regression at frameCapture-constructionOwnership.test.ts:107-148 exercises the actual never-resolving browser.close() path and proves the graceful attempt is followed by forced disconnect; the suite teardown also could not complete if the pool close promise remained stuck.
The earlier fixes remain intact: sole-owner releaseBrowser(browser) compatibility is preserved, ambiguous multi-owner handle release fails rather than consuming another lease, Studio retains exact thumbnail lease identity, and construction rollback applies the guarded page/browser cleanup on every failure path. I also re-enumerated production acquireBrowser/release sites; frame capture and Studio both retain and release the exact lease.
All exact-head required checks are green, including the eight regression shards, aggregate regression, Windows, Producer, CodeQL, and Graphite mergeability.
Important, non-blocking docs follow-up: packages/engine/src/index.ts:20-23 still says cleanup functions “never throw” and names releaseBrowser, but ambiguous handle release now intentionally rejects. The new behavior is the safe one; please update that error-convention comment so the public contract matches it.
Verdict: APPROVE
Reasoning: Exact lease ownership, hung-close escalation, multi-owner safety, and cleanup settlement are now coherent and regression-covered; no resource-lifetime blocker remains.
— Magi

What
Replace partial browser pooling with immutable, fingerprinted browser leases.
Why
Acquisition could observe the wrong launch configuration or race a browser that was already closing, and construction failure could leak ownership.
How
Model launching/ready/closing entries, remove closing instances from availability, return idempotent leases, and roll back partial sessions.
Test plan